Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

test: replace port in dgram cb test #12942

Closed
wants to merge 1 commit into from
Closed

test: replace port in dgram cb test #12942

wants to merge 1 commit into from

Conversation

arturgvieira-zz
Copy link

@arturgvieira-zz arturgvieira-zz commented May 10, 2017 •
edited
Loading

Replaced common.PORT in the following test.
test-dgram-send-callback-buffer.js

Ref: #12376

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

test dgram

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label May 10, 2017
@mscdex mscdex added the dgram Issues and PRs related to the dgram subsystem / UDP. label May 10, 2017
@Trott
Copy link
Member

Trott commented May 10, 2017

It would be great if your commit messages and PR titles could be less generic. Having multiple PRs with identical titles makes it really hard to track what's going on. Including the test name would be helpful.

@arturgvieira-zz
Copy link
Author

arturgvieira-zz commented May 10, 2017 •
edited
Loading

You got it, will change the commit messages and name of the PRs. Thanks

@arturgvieira-zz arturgvieira-zz changed the title test: remove common.PORT from test test: replace port in dgram cb test May 10, 2017
@arturgvieira-zz
Copy link
Author

@Trott updated the PR titles and commit message headings. Let me know your thoughts.

@Trott
Copy link
Member

Trott commented May 10, 2017

@Trott updated the PR titles and commit message headings. Let me know your thoughts.

Thanks. That's much easier to differentiate in notifications.

@arturgvieira-zz
Copy link
Author

Thank you. I am still new and appreciate learning best practices. Thanks for taking the time to explain.

cjihrig
cjihrig requested changes May 10, 2017
View reviewed changes
test/parallel/test-dgram-send-callback-buffer.js Outdated
@@ -14,4 +14,14 @@ const onMessage = common.mustCall(function(err, bytes) {
client.close();
});

client.send(buf, common.PORT, common.localhostIPv4, onMessage);
const portGetter = dgram.createSocket('udp4')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could just use the existing client socket here.

@arturgvieira-zz arturgvieira-zz changed the title test: replace port in dgram cb test test: move to sequential dgram cb test May 10, 2017
@arturgvieira-zz
Copy link
Author

arturgvieira-zz commented May 11, 2017 •
edited
Loading

@cjihrig If you could show me how to use the client socket here instead I would appreciate it from an understanding perspective. It turns out that currently it is preferred to remove the change and instead keep the original tests. #12929

@cjihrig
Copy link
Contributor

cjihrig commented May 11, 2017

If you could show me how to use the client socket here instead I would appreciate it from an understanding perspective.

Call bind() on the existing client socket. After binding, use client.address() to get the port used by send().

@arturgvieira-zz
Copy link
Author

arturgvieira-zz commented May 11, 2017 •
edited
Loading

Oh, I see, thanks for the help.

@lpinca
Copy link
Member

lpinca commented May 11, 2017

For anyone reviewing, see discussion in #12929.

@arturgvieira-zz arturgvieira-zz changed the title test: move to sequential dgram cb test test: replace port in dgram cb test May 11, 2017
lpinca
lpinca reviewed May 12, 2017
View reviewed changes
test/parallel/test-dgram-send-callback-buffer.js Outdated
@@ -14,4 +14,4 @@ const onMessage = common.mustCall(function(err, bytes) {
client.close();
});

client.send(buf, common.PORT, common.localhostIPv4, onMessage);
client.bind(0, () => client.send(buf, client.address().port, common.localhostIPv4, onMessage));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is too long, the linter will not like it.

@arturgvieira-zz
Copy link
Author

@lpinca All done, wrapped arguments to shorten line length.

lpinca
lpinca reviewed May 12, 2017
View reviewed changes
test/parallel/test-http-server-de-chunked-trailer.js Outdated
@@ -0,0 +1,23 @@
'use strict';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has this new test been added by mistake?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'm sorry my mistake, give me a second and I will correct that.

@lpinca
Copy link
Member

lpinca commented May 12, 2017 •
edited
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/8033/
CI: https://ci.nodejs.org/job/node-test-pull-request/8037/

test: replace port in dgram cb test
b53b1ac 
Replaced common.PORT in the following test.
test-dgram-send-callback-buffer.js

Ref: #12376
mhdawson
mhdawson approved these changes May 12, 2017
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lpinca
Copy link
Member

lpinca commented May 22, 2017

Landed in e00116d.

@lpinca lpinca closed this May 22, 2017
lpinca pushed a commit that referenced this pull request May 22, 2017
@lpinca
test: use dynamic port in test-dgram-send-callback-buffer
e00116d 
Replace common.PORT with available port assigned by the operating
system in test-dgram-send-callback-buffer.

PR-URL: #12942
Refs: #12376
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@Trott Trott mentioned this pull request May 22, 2017
Closed
64 tasks
jasnell pushed a commit that referenced this pull request May 23, 2017
@jasnell
test: use dynamic port in test-dgram-send-callback-buffer
4b489e7 
Replace common.PORT with available port assigned by the operating
system in test-dgram-send-callback-buffer.

PR-URL: #12942
Refs: #12376
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
jasnell pushed a commit that referenced this pull request May 23, 2017
@jasnell
test: use dynamic port in test-dgram-send-callback-buffer
8ef4fe0 
Replace common.PORT with available port assigned by the operating
system in test-dgram-send-callback-buffer.

PR-URL: #12942
Refs: #12376
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@jasnell jasnell mentioned this pull request May 28, 2017
Closed
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
Closed
3 tasks
MylesBorins pushed a commit that referenced this pull request Jul 17, 2017
@MylesBorins
test: use dynamic port in test-dgram-send-callback-buffer
7909c6d 
Replace common.PORT with available port assigned by the operating
system in test-dgram-send-callback-buffer.

PR-URL: #12942
Refs: #12376
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jul 18, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lpinca lpinca lpinca approved these changes

@mhdawson mhdawson mhdawson approved these changes

@cjihrig cjihrig cjihrig approved these changes

Assignees
No one assigned
Labels
dgram Issues and PRs related to the dgram subsystem / UDP. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@arturgvieira-zz @Trott @cjihrig @lpinca @mhdawson @mscdex @MylesBorins @nodejs-github-bot